[fix](fe) Prevent pushing other join conditions to right child of null-aware anti join when hash conjuncts are empty#64898
Open
starocean999 wants to merge 2 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary for PR #64898:
I reviewed the three changed files and the surrounding Nereids null-aware anti join rewrite/translation path. I did not find a substantiated blocking issue, so I am leaving this as a summary-only review with no inline comments.
Critical checkpoint conclusions:
- Goal and proof: the PR prevents
PushDownJoinOtherConditionfrom pushing a right-child-only predicate below aNULL_AWARE_LEFT_ANTI_JOINwhenhashJoinConjunctsis empty. The addedqt_select65covers the constant-vs-null subquery case and expectsCOUNT(*) = 0. - Scope: the code change is small and focused on the affected pushdown rule, with a matching regression output update.
- Parallel paths: checked
InApplyToJoin,FindHashConditionForJoin,JoinUtils.JoinSlotCoverageChecker, repeatedRuleSet.PUSH_DOWN_FILTERSuse, and physical hash/nested-loop translation. I did not find another path that would still push the same no-hash NAAJ right-only comparison into the build child. - Session/config behavior: no new configuration item is added. The regression suite does not set planner variables, but
SessionVariabledefaults keep Nereids enabled and fallback disabled, so the new case should exercise the Nereids rule path under default regression settings. - Concurrency, lifecycle, persistence, data writes, FE-BE protocol compatibility, and observability: not materially involved by this PR.
- Test/output review: the new query returns a single aggregate row, so ordering is deterministic, and the
.outlabel/result matches the suite case. - Validation limit: I did not run FE build or the regression suite locally because this runner lacks
thirdparty/installedandthirdparty/installed/bin/protoc. I did rungit diff --checkon the three PR files, which passed.
User focus: no additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewritereportedNO_NEW_VALUABLE_FINDINGS; no optimizer candidate became an inline comment.tests-session-configreportedNO_NEW_VALUABLE_FINDINGS; no test/session candidate became an inline comment.- Convergence round 1 ended with both live subagents reporting
NO_NEW_VALUABLE_FINDINGSfor the same current ledger and empty proposed final inline comment set.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29036 ms |
Contributor
TPC-DS: Total hot run time: 171437 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
starocean999
requested review from
924060929,
englefly and
morrySnow
as code owners
Contributor
|
add related PR |
…l-aware anti join when hash conjuncts are empty
starocean999
force-pushed
the
master_0626
branch
from
09ffce1 to
e8a6b55
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review summary: I reviewed the changed rule and regression coverage and did not find a new blocking issue.
Critical checkpoints:
- Goal and proof: the PR prevents right-side
otherJoinConjunctpushdown for hash-emptyNULL_AWARE_LEFT_ANTI_JOIN, preservingNOT INsemantics when the subquery side contains NULL. The addedqt_select65regression covers1 NOT IN (SELECT CAST(NULL AS INT))and expects count0. - Scope and clarity: the code change is narrow and now includes the explanatory comment requested in the existing inline thread.
- Parallel paths: checked
InApplyToJoin,FindHashConditionForJoin,PushDownFilterThroughJoin,JoinExtractOrFromCaseWhen, nested-loop translation, and null-aware nested-loop handling; I did not find another path that reintroduces the same premature right-side filtering. - Concurrency, lifecycle, configs, persistence, FE/BE protocol compatibility, and observability: not materially touched by this PR.
- Test/output review: the added result is deterministic.
git diff --checkis clean for the PR range.
Validation limit: I did not run the targeted regression suite or FE build because this runner is missing .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc.
User focus: no additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite and tests-session-config both found no candidate inline comments in their initial passes. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same zero-inline final comment set.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29464 ms |
Contributor
TPC-DS: Total hot run time: 173244 ms |
Contributor
ClickBench: Total hot run time: 25.3 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
PushDownJoinOtherConditionprocesses aNULL_AWARE_LEFT_ANTI_JOINwith empty hash join conjuncts, it incorrectly pushes the other join conditions
that reference only the right-side slots down to the right child. This changes
the evaluation semantics of the NOT IN subquery, because filtering the right
child before the null-aware anti join eliminates NULL values prematurely.
For a
NULL_AWARE_LEFT_ANTI_JOIN, the hash join conjuncts represent thecorrelation between the outer and inner tables. When there is no correlation
(i.e., hash conjuncts are empty), the other join conditions must not be pushed
to the right child, as doing so would filter rows from the subquery result
before the NULL check, potentially producing incorrect results.
Example affected query:
SELECT COUNT(*) FROM (SELECT 1 AS x) t
WHERE 1 NOT IN (SELECT CAST(NULL AS INT));
The subquery returns NULL, so NOT IN should evaluate to UNKNOWN (NULL),
meaning no rows match and COUNT returns 0. However, pushing the other condition
to the right child changes this behavior and produces wrong results.
Fix:
Add a guard in
PushDownJoinOtherConditionto keep the other condition asa remaining other conjunct (not pushed to the right child) when the join type
is
NULL_AWARE_LEFT_ANTI_JOINand hash join conjuncts are empty.Related PR: #16774
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)